Skip to content

[codex] default desktop chat transport to websocket#121

Merged
zouyonghe merged 9 commits intoAstrBotDevs:mainfrom
zouyonghe:codex/default-websocket
Apr 16, 2026
Merged

[codex] default desktop chat transport to websocket#121
zouyonghe merged 9 commits intoAstrBotDevs:mainfrom
zouyonghe:codex/default-websocket

Conversation

@zouyonghe
Copy link
Copy Markdown
Member

@zouyonghe zouyonghe commented Apr 16, 2026

Summary

This change makes the desktop shell default new chat sessions to the websocket transport instead of SSE.

Issue #120 reported that desktop chat felt much slower than direct OpenAI streaming. The desktop shell loads the packaged AstrBot ChatUI, which currently defaults to SSE unless chat.transportMode is already present in local storage. On the upstream side, the SSE chat route adds a fixed await asyncio.sleep(0.05) after each streamed payload, so new desktop installs inherit that slower transport by default even though the same ChatUI already supports websocket.

This patch keeps the change scoped to desktop startup. The injected desktop bridge now seeds localStorage["chat.transportMode"] = "websocket" only when the key is missing, so existing users keep their chosen setting while fresh desktop installs start on the lower-latency websocket path.

Validation

  • node --check src-tauri/src/bridge_bootstrap.js
  • cargo test --manifest-path src-tauri/Cargo.toml

Summary by Sourcery

Default desktop chat sessions to the websocket chat transport by seeding a shared, contract-driven localStorage setting during desktop bridge startup.

New Features:

  • Seed a default websocket chat transport preference in desktop ChatUI via the injected desktop bridge, without overriding existing user settings.

Enhancements:

  • Introduce a shared JSON contract for chat transport storage keys and values to keep the Rust desktop bridge and JS bootstrap in sync.
  • Extend desktop bridge expectation checks to verify that chat components read and write the chat transport preference consistently.

Tests:

  • Add tests to validate that the bridge bootstrap script uses the shared chat transport contract placeholders and that desktop bridge expectations cover chat transport preference access.

github-actions bot and others added 5 commits April 11, 2026 14:44
* fix: add backend startup heartbeat liveness probe

* fix: tighten startup heartbeat validation

* refactor: centralize startup heartbeat metadata

* fix: surface heartbeat invalidation sooner

* fix: harden startup heartbeat parsing

* fix: warn on stop-time heartbeat failures

* refactor: simplify startup heartbeat control flow

* refactor: flatten readiness heartbeat helpers

* refactor: clarify heartbeat helper responsibilities

* docs: clarify startup heartbeat path coupling

* fix: harden startup heartbeat coordination

* fix: make startup heartbeat checks monotonic

* fix: clean up heartbeat test and exit handling
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a default chat transport mode setting in the desktop bridge. It adds constants for the storage key and the 'websocket' mode, implements a function to ensure this default is set in local storage if not already present, and integrates this check into the bootstrap process. I have no feedback to provide as there are no review comments.

@zouyonghe zouyonghe marked this pull request as ready for review April 16, 2026 03:03
Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • In ensureDefaultChatTransportMode, consider explicitly checking storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== null instead of a truthy check so that an intentionally stored empty string or falsy value is still respected as an existing preference.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `ensureDefaultChatTransportMode`, consider explicitly checking `storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== null` instead of a truthy check so that an intentionally stored empty string or falsy value is still respected as an existing preference.

## Individual Comments

### Comment 1
<location path="src-tauri/src/bridge_bootstrap.js" line_range="704-707" />
<code_context>
+    try {
+      const storage = window.localStorage;
+      if (!storage) return;
+      if (storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY)) return;
+      storage.setItem(
+        CHAT_TRANSPORT_MODE_STORAGE_KEY,
</code_context>
<issue_to_address>
**suggestion:** Guard should distinguish between `null` and other falsy stored values to avoid unintended overrides.

Because this relies on truthiness, valid stored values like `""` or `"0"` will be treated as missing and overwritten with the default. If you only want to set the default when the key is absent, check explicitly for `null`, e.g. `if (storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== null) return;`.

```suggestion
      const storage = window.localStorage;
      if (!storage) return;
      if (storage.getItem(CHAT_TRANSPORT_MODE_STORAGE_KEY) !== null) return;
      storage.setItem(
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src-tauri/src/bridge_bootstrap.js Outdated
@zouyonghe
Copy link
Copy Markdown
Member Author

@sourcery-ai review

Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The ensureDefaultChatTransportMode function silently swallows all errors; consider at least logging unexpected exceptions or narrowing the try/catch to make debugging startup/localStorage issues easier.
  • The CHAT_TRANSPORT_MODE_STORAGE_KEY and CHAT_TRANSPORT_MODE_WEBSOCKET constants are hard-coded here; if the ChatUI or upstream code defines equivalent values, it may be safer to centralize or reference them to avoid divergence if the key name or mode string changes later.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `ensureDefaultChatTransportMode` function silently swallows all errors; consider at least logging unexpected exceptions or narrowing the try/catch to make debugging startup/localStorage issues easier.
- The `CHAT_TRANSPORT_MODE_STORAGE_KEY` and `CHAT_TRANSPORT_MODE_WEBSOCKET` constants are hard-coded here; if the ChatUI or upstream code defines equivalent values, it may be safer to centralize or reference them to avoid divergence if the key name or mode string changes later.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe
Copy link
Copy Markdown
Member Author

@sourcery-ai review

@zouyonghe
Copy link
Copy Markdown
Member Author

Addressed in bf15fb9:

  • narrowed ensureDefaultChatTransportMode localStorage handling so read/write failures are logged via devWarn instead of being swallowed silently
  • replaced the standalone transport literals in the desktop bridge with a documented shared transport contract object
  • added prepare-resources compatibility expectations so packaging now verifies upstream ChatUI still uses localStorage["chat.transportMode"] and recognizes "websocket" before we ship the desktop patch

Validation used for this update:

  • node --check src-tauri/src/bridge_bootstrap.js
  • pnpm run test:prepare-resources
  • cargo test --manifest-path src-tauri/Cargo.toml

Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

  • The chat.transportMode key and websocket value are now hard-coded both in desktop-bridge-expectations.mjs and bridge_bootstrap.js; consider centralizing these constants (or at least deriving one from the other) to avoid subtle drift if the storage contract changes in the future.
  • In ensureDefaultChatTransportMode, you might want to defensively check for typeof window !== 'undefined' before accessing window.localStorage so this bootstrap module fails more gracefully if ever imported or executed in a non-browser-like context (e.g., certain tests or tooling).
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `chat.transportMode` key and `websocket` value are now hard-coded both in `desktop-bridge-expectations.mjs` and `bridge_bootstrap.js`; consider centralizing these constants (or at least deriving one from the other) to avoid subtle drift if the storage contract changes in the future.
- In `ensureDefaultChatTransportMode`, you might want to defensively check for `typeof window !== 'undefined'` before accessing `window.localStorage` so this bootstrap module fails more gracefully if ever imported or executed in a non-browser-like context (e.g., certain tests or tooling).

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe
Copy link
Copy Markdown
Member Author

Addressed in e42f8c0:

  • moved the desktop chat transport storage contract into src-tauri/src/desktop_bridge_chat_transport_contract.json so the injected bridge and prepare-resources compatibility checks derive from the same source
  • updated the Rust bridge injector to replace transport placeholders from that shared contract at eval time
  • added an early typeof window === "undefined" guard so the bootstrap exits cleanly in non-browser execution contexts

Validation for this update:

  • node --check src-tauri/src/bridge_bootstrap.js
  • pnpm run test:prepare-resources
  • cargo test --manifest-path src-tauri/Cargo.toml

@zouyonghe
Copy link
Copy Markdown
Member Author

@sourcery-ai review

Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • In desktop_bridge_bootstrap_script, the event_name parameter is effectively only honored on the first call because of the OnceLock, so either remove the parameter or document/enforce that it must be constant to avoid future misuse where different event names are silently ignored.
  • The new desktop-bridge-expectations hints still hard-code localStorage["chat.transportMode"] and "websocket" while the actual key/value now come from desktop_bridge_chat_transport_contract.json; consider reading the contract for the hint strings as well to avoid divergence if the contract changes.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `desktop_bridge_bootstrap_script`, the `event_name` parameter is effectively only honored on the first call because of the `OnceLock`, so either remove the parameter or document/enforce that it must be constant to avoid future misuse where different event names are silently ignored.
- The new `desktop-bridge-expectations` hints still hard-code `localStorage["chat.transportMode"]` and `"websocket"` while the actual key/value now come from `desktop_bridge_chat_transport_contract.json`; consider reading the contract for the hint strings as well to avoid divergence if the contract changes.

## Individual Comments

### Comment 1
<location path="src-tauri/src/bridge_bootstrap.js" line_range="713-714" />
<code_context>
     } catch {}
   };

+  const ensureDefaultChatTransportMode = () => {
+    const storage = window.localStorage;
+    if (!storage) return;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `window.localStorage` can throw before the try/catch blocks run.

In some environments (e.g., storage disabled), reading `window.localStorage` can itself throw, skipping your existing `try/catch` around `getItem`/`setItem` and potentially aborting bootstrap. Please either wrap the `window.localStorage` access in its own `try/catch` or move it into the existing `try` blocks so failures are handled consistently and only produce a logged warning.
</issue_to_address>

### Comment 2
<location path="src-tauri/src/bridge/desktop.rs" line_range="22-27" />
<code_context>
+    websocket_value: String,
+}
+
+fn desktop_bridge_chat_transport_contract() -> &'static DesktopBridgeChatTransportContract {
+    DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT.get_or_init(|| {
+        serde_json::from_str(DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT_TEMPLATE)
+            .expect("desktop bridge chat transport contract must be valid JSON")
+    })
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The Rust-side contract loading doesn’t enforce the non-empty-string constraint that the JS tooling validates.

Node validates `desktop_bridge_chat_transport_contract.json` to ensure `storageKey` and `websocketValue` are non-empty, but this loader only checks that the JSON parses. If the file is modified or generated outside the Node path, empty keys/values could be injected into the bootstrap script without any Rust-side guard. Please add equivalent non-empty checks here (e.g., `assert!(!storage_key.is_empty() && !websocket_value.is_empty())`) so contracts fail fast when invalid.

```suggestion
fn desktop_bridge_chat_transport_contract() -> &'static DesktopBridgeChatTransportContract {
    DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT.get_or_init(|| {
        let contract: DesktopBridgeChatTransportContract =
            serde_json::from_str(DESKTOP_BRIDGE_CHAT_TRANSPORT_CONTRACT_TEMPLATE)
                .expect("desktop bridge chat transport contract must be valid JSON");

        assert!(
            !contract.storage_key.is_empty(),
            "desktop bridge chat transport contract storageKey must be non-empty"
        );
        assert!(
            !contract.websocket_value.is_empty(),
            "desktop bridge chat transport contract websocketValue must be non-empty"
        );

        contract
    })
}
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread src-tauri/src/bridge_bootstrap.js Outdated
Comment thread src-tauri/src/bridge/desktop.rs
@zouyonghe
Copy link
Copy Markdown
Member Author

Addressed in d22db28:

  • removed the injectable event_name parameter from the desktop bridge bootstrap path so the OnceLock can no longer silently ignore future alternate values; the bridge now always binds to the single TRAY_RESTART_BACKEND_EVENT constant
  • wrapped window.localStorage acquisition in its own guarded read so storage-disabled environments now log and bail instead of throwing before the existing warning path
  • derived the desktop bridge expectation hints from the shared transport contract values instead of re-hardcoding chat.transportMode / websocket in the hint text
  • added Rust-side non-empty assertions when loading the shared chat transport contract so invalid JSON values fail fast outside the Node tooling path too

Validation for this update:

  • node --check src-tauri/src/bridge_bootstrap.js
  • pnpm run test:prepare-resources
  • cargo test --manifest-path src-tauri/Cargo.toml

@zouyonghe
Copy link
Copy Markdown
Member Author

@sourcery-ai review

Copy link
Copy Markdown

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Copy link
Copy Markdown

@SourceryAI SourceryAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Hi @zouyonghe! 👋

Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀

Install the sourcery-ai bot to get automatic code reviews on every pull request ✨

Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zouyonghe zouyonghe merged commit 8b78c97 into AstrBotDevs:main Apr 16, 2026
4 checks passed
zouyonghe added a commit to zouyonghe/AstrBot-desktop that referenced this pull request Apr 17, 2026
* chore(version): sync desktop version to v4.23.0-beta.1

* fix: add backend startup heartbeat liveness probe (AstrBotDevs#114)

* fix: add backend startup heartbeat liveness probe

* fix: tighten startup heartbeat validation

* refactor: centralize startup heartbeat metadata

* fix: surface heartbeat invalidation sooner

* fix: harden startup heartbeat parsing

* fix: warn on stop-time heartbeat failures

* refactor: simplify startup heartbeat control flow

* refactor: flatten readiness heartbeat helpers

* refactor: clarify heartbeat helper responsibilities

* docs: clarify startup heartbeat path coupling

* fix: harden startup heartbeat coordination

* fix: make startup heartbeat checks monotonic

* fix: clean up heartbeat test and exit handling

* fix: default desktop chat transport to websocket

* fix: respect existing desktop transport preference

* fix: harden desktop transport bootstrap

* fix: centralize desktop transport contract

* fix: harden desktop bridge transport injection

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants